feat: mdoc-support #2054

auer-martin · 2024-10-08T13:50:25Z

No description provided.

changeset-bot · 2024-10-08T13:50:31Z

🦋 Changeset detected

Latest commit: 95eb0db

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages

Name	Type
@credo-ts/core	Patch
@credo-ts/action-menu	Patch
@credo-ts/anoncreds	Patch
@credo-ts/askar	Patch
@credo-ts/bbs-signatures	Patch
@credo-ts/cheqd	Patch
@credo-ts/drpc	Patch
@credo-ts/indy-sdk-to-askar-migration	Patch
@credo-ts/indy-vdr	Patch
@credo-ts/node	Patch
@credo-ts/openid4vc	Patch
@credo-ts/question-answer	Patch
@credo-ts/react-native	Patch
@credo-ts/tenants	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Signed-off-by: Martin Auer <[email protected]>

TimoGlastra · 2024-10-08T15:47:27Z

packages/openid4vc/src/openid4vc-holder/OpenId4vcSiopHolderService.ts

+ responseUri:
+ authorizationRequest.authorizationRequestPayload.response_uri ??
+ authorizationRequest.authorizationRequestPayload.response_uri,


this is redundant

TimoGlastra · 2024-10-08T15:48:29Z

packages/openid4vc/src/openid4vc-holder/OpenId4vcSiopHolderService.ts

@@ -109,6 +110,14 @@ export class OpenId4VcSiopHolderService {
 challenge: nonce,
 domain: clientId,
 presentationSubmissionLocation: DifPresentationExchangeSubmissionLocation.EXTERNAL,
+ openid4vp: {


how are these values mapped to the presentation? Ideally we don't add oid4vp specific properties to the pex service, but i can understand if it's not possible otherwise

Yeah, I also don't like it, but I don't see an easy better way to propagate the values. There are used to calculate the session transcript

okay and this is openid4vp specific, or is it mdoc specific?

Hmm I guess depends on the viewpoint the data we need is expected to come for openid4vp, but we need it to create mdoc presentations.

TimoGlastra · 2024-10-08T15:49:09Z

packages/openid4vc/src/openid4vc-holder/OpenId4vcSiopHolderService.ts

@@ -109,6 +110,14 @@ export class OpenId4VcSiopHolderService {
 challenge: nonce,
 domain: clientId,
 presentationSubmissionLocation: DifPresentationExchangeSubmissionLocation.EXTERNAL,
+ openid4vp: {
+ clientId,
+ verifierGeneratedNonce: nonce,


I think this should be extracted from the challenge property in the pex service itself. As it's now redundant, and can lead to inconsistencies.

I did this intentionally. I don't think we should use a challenge property. openid4vp is very specific. To me it would be completely unclear why the verification of a device response will fail after changing a e.g. domain/challenge property which have no relation at all to Mdoc?

What do you think?

Hmm I think it's quite common to use one nonce and apply that in all places that need a nonce. That way you keep consistency. we use the presentation exchange nonce in the w3c vp signature creation, so i don't see why we can't use it in the mdoc presentation creation.

I rather reuse the existing property, as there's really no reason to use a different nonce for the mdoc presentation.

because the nonce for a w3c jwt vp / sd-jwt-vc is also based on the challenge. We just call it challenge in our api, but all VPs use this as the nonce/challenge.

TimoGlastra · 2024-10-08T15:50:00Z

packages/openid4vc/src/openid4vc-holder/OpenId4vcSiopHolderService.ts

+ openid4vp: {
+ clientId,
+ verifierGeneratedNonce: nonce,
+ mdocGeneratedNonce: await agentContext.wallet.generateNonce(),


we need to make sure to reuse this nonce when encrypting the resposne (we generate a new nonce there at the moment, but these should be the same)

TimoGlastra · 2024-10-08T15:50:17Z

packages/openid4vc/src/shared/transform.ts

@@ -41,6 +45,8 @@ export function getSphereonVerifiablePresentation(
 return JsonTransformer.toJSON(verifiablePresentation) as SphereonW3cVerifiablePresentation
 } else if (verifiablePresentation instanceof W3cJwtVerifiablePresentation) {
 return verifiablePresentation.serializedJwt
+ } else if (verifiablePresentation instanceof MdocVerifiablePresentation) {
+ throw new CredoError('Mdoc verifiable credential is not yet supported.')


Suggested change

throw new CredoError('Mdoc verifiable credential is not yet supported.')

throw new CredoError('Mdoc verifiable presentation is not yet supported.')

TimoGlastra · 2024-10-08T15:51:39Z

packages/core/src/modules/dif-presentation-exchange/DifPresentationExchangeService.ts

+
+ const { deviceResponseBase64Url, presentationSubmission } = await MdocDeviceResponse.openId4Vp(agentContext, {
+ mdocs: [Mdoc.fromBase64Url(mdocRecord.base64Url)],
+ presentationDefinition: presentationDefinition as DifPresentationExchangeDefinitionV2,


what if v1 is used?

TimoGlastra · 2024-10-08T15:53:09Z

packages/core/src/modules/dif-presentation-exchange/DifPresentationExchangeService.ts

+ claimFormat: presentationToCreate.claimFormat,
+ })
+
+ continue


I think instead of continue we could use an else clause maybe? I had a hard time understanding this code, until i spotted this continue statement

TimoGlastra · 2024-10-08T15:55:12Z

packages/core/src/modules/dif-presentation-exchange/utils/credentialSelection.ts

+ .filter((c) => c instanceof MdocRecord === false)
+ .map((c) => getSphereonOriginalVerifiableCredential(c as SdJwtVcRecord | W3cCredentialRecord))


Suggested change

.filter((c) => c instanceof MdocRecord === false)

.map((c) => getSphereonOriginalVerifiableCredential(c as SdJwtVcRecord | W3cCredentialRecord))

.filter((c): c is Exclude<typeof c, MdocRecord> => c instanceof MdocRecord === false)

.map((c) => getSphereonOriginalVerifiableCredential(c))

TimoGlastra · 2024-10-08T15:57:30Z

packages/core/src/modules/dif-presentation-exchange/utils/credentialSelection.ts

 ...selectResultsRaw,
+ areRequiredCredentialsPresent:
+ mdocPresentationDefinition.input_descriptors.length >= 1
+ ? 'warn' // we don't know yet wheater the required credentials are present


when will we know this? (i don't see warn changed to error / info?)

TimoGlastra · 2024-10-08T15:59:41Z

packages/core/src/modules/mdoc/Mdoc.ts

+ public get deviceSignedNamespaces(): MdocNameSpaces {
+ if (this.issuerSignedDocument instanceof DeviceSignedDocument === false) {
+ throw new MdocError(`Cannot get 'device-namespaces from a IssuerSignedDocument. Must be a DeviceSignedDocument.`)
+ }
+
+ return this.issuerSignedDocument.allDeviceSignedNamespaces
+ }


isn't device signed the presentation?

yes. It can also include device-signed namespaces. Essentially self-attested claims.

but shouldn't this be on the mdoc presentation then?

TimoGlastra · 2024-10-08T16:01:12Z

packages/core/src/modules/mdoc/Mdoc.ts

+ document.addIssuerNameSpace(namespace, namespaceRecord)
+ }
+
+ const cert = X509Certificate.fromEncodedCertificate(issuerCertificate)


i see we're mostly using string certificates here. Do we want to use that as the primary method to pass around certificate in Credo (i haven't made my mind up yet, just thinking 🤔 ).

I don't mind. We essentially also set trustedCerts as strings. That's why I have chosen this approach

export interface X509ModuleConfigOptions { /** * * Array of trusted base64-encoded certificate strings in the DER-format. */ trustedCertificates?: [string, ...string[]]

string is fine 👍

TimoGlastra · 2024-10-08T16:01:37Z

packages/core/src/modules/mdoc/Mdoc.ts

+ }
+
+ const cert = X509Certificate.fromEncodedCertificate(issuerCertificate)
+ const issuerPrivateJwk = await getJwkFromKey(options.issuerKey ?? cert.publicKey)


Do we check if the issuerKey and cert key match? Can't we always just extract the key from the cert?

I also think this should work. Not sure why it doesn't. It may be related to the issue I mentioned below with the jwk representation of keys.

TimoGlastra · 2024-10-08T16:02:29Z

packages/core/src/modules/mdoc/Mdoc.ts

+ const issuerPrivateJwk = await getJwkFromKey(options.issuerKey ?? cert.publicKey)
+ const issuerSignedDocument = await document.sign(
+ {
+ issuerPrivateKey: issuerPrivateJwk.toJson(),


this is the public key jwk, not the private key jwk right? I think the name is confusing. It's important we don't have to pass the private key, so we can support HSM

TimoGlastra · 2024-10-08T16:03:27Z

packages/core/src/modules/mdoc/Mdoc.ts

+ const issuerSignedDocument = await document.sign(
+ {
+ issuerPrivateKey: issuerPrivateJwk.toJson(),
+ alg: issuerPrivateJwk.supportedSignatureAlgorithms[0] as 'ES256' | 'ES384' | 'ES512' | 'EdDSA',


I don't like the casting here, it can lead to bugs in the future. We should check if index 0 exists, and then also whether it's a supported alg by mdoc (or does it throw if the provided alg is not supported?)

TimoGlastra · 2024-10-08T16:05:12Z

packages/core/src/modules/mdoc/Mdoc.ts

+ return new Mdoc(issuerSignedDocument)
+ }
+
+ public async verify(agentContext: AgentContext, options?: MdocVerifyOptions): Promise<boolean> {


just as an FYI trusted certificaes will need to be aligned with #2052 once merged

TimoGlastra · 2024-10-08T16:06:19Z

packages/core/src/modules/mdoc/Mdoc.ts

+ getMdocContext(agentContext)
+ )
+
+ await verifier.verifyData({ mdoc: this.issuerSignedDocument }, mdocContext)


does it return a verification object or something? Would be good to return more than true/false, but also something we can add later

TimoGlastra · 2024-10-08T16:07:11Z

packages/core/src/modules/mdoc/MdocApi.ts

+ * @param options {MdocCreateOptions}
+ * @returns {Promise<Mdoc>}
+ */
+ public async create(options: MdocCreateOptions) {


does create also sign? I think in SD JWT API we use sign so might be nice to stay consistent?

TimoGlastra · 2024-10-08T16:11:45Z

packages/core/src/modules/mdoc/MdocContext.ts

+ const { mac0, jwk, options } = input
+ const { data, signature, alg } = mac0.getRawVerificationData(options)
+ return await verifyWithJwk({ jwk, signature, data, alg })


does this still use the credo wallet to verify the signature? I can't see the context or something passed around. It is important.

Aslo it's prob ok for now, but we should try to stay away from doing crypto operations outside of the wallet. Also with e.g. the p256 shared secret calcuation etc.. The more we can do in the wallet, the better.

So if you can provide a list of all cryptographic operations that are needed, we can look to move them to the wallet over time

We just need the key derivation.

TimoGlastra · 2024-10-08T16:12:06Z

packages/core/src/modules/mdoc/MdocContext.ts

+ },
+ getPublicKey: async (input) => {
+ const certificate = new x509.X509Certificate(input.certificate)
+ const key = await importX509({


what do these do? and why are they part of the protokol library?

Please take a look at my comment. There is an issue with how we represent the key as jwk. It probably should be compressed. Nothing from the protokoll library should remain here. Atleast related to crypto.

Which comment? I can't find it

//expected //{ //kty: 'EC', //x: 'OFBq4YMKg4w5fTifsytwBuJf_7E7VhRPXiNm52S3q1E', //y: 'EyIAXV8gyt5FcRsYHhz4ryz97rjL0uogxHO6jMZr3bg', //crv: 'P-256' //} //actual //{ //kty: 'EC', //crv: 'P-256', //x: 'OFBq4YMKg4w5fTifsytwBuJf_7E7VhRPXiNm52S3q1ETIgBdXyDK3kVxGxgeHPiv', //y: 'LP3uuMvS6iDEc7qMxmvduNeBp_oWscK1x-3_1KKYDayIctdDcpXHi8HcbehAfVIK' //} //const comp = X509Certificate.fromRawCertificate(input.certificate) //const x = getJwkFromKey(comp.publicKey).toJson() //// eslint-disable-next-line @typescript-eslint/no-unused-vars //const { use, ...jwk } = x //return jwk

TimoGlastra · 2024-10-08T16:14:17Z

packages/core/src/modules/mdoc/MdocDeviceResponse.ts

+
+ const publicDeviceJwk = COSEKey.import(deviceKeyInfo.deviceKey).toJWK()
+
+ deviceKeyInfo.deviceKey


Suggested change

deviceKeyInfo.deviceKey

TimoGlastra · 2024-10-08T16:14:48Z

packages/core/src/modules/mdoc/MdocDeviceResponse.ts

+ return {
+ deviceResponseBase64Url: TypedArrayEncoder.toBase64URL(deviceResponseMdoc.encode()),
+ presentationSubmission: MdocDeviceResponse.createPresentationSubmission({
+ id: 'MdocPresentationSubmission ' + agentContext.wallet.generateNonce(),


I think we can also just use an uuid?

Suggested change

id: 'MdocPresentationSubmission ' + agentContext.wallet.generateNonce(),

id: 'MdocPresentationSubmission ' + await agentContext.wallet.generateNonce(),

TimoGlastra · 2024-10-08T16:15:12Z

packages/core/src/modules/mdoc/MdocDeviceResponse.ts

+ const trustedCerts =
+ options?.trustedCertificates ?? agentContext.dependencyManager.resolve(X509ModuleConfig).trustedCertificates
+
+ if (!trustedCerts) {
+ throw new MdocError('No trusted certificates found. Cannot verify mdoc.')


same with integraiont of pr #2052

… test

Signed-off-by: Martin Auer <[email protected]>

TimoGlastra · 2024-10-11T10:31:59Z

packages/core/src/modules/x509/X509ServiceOptions.ts

@@ -22,6 +22,7 @@ export interface X509CreateSelfSignedCertificateOptions {
 notBefore?: Date
 notAfter?: Date
 name?: string


I think name can just be a string with CN=X, N=X I think that might be nicer to keep it like that. Otherwise we'll have to add a custom prop for everything

or we need to nest / rename it

TimoGlastra · 2024-10-11T10:41:19Z

packages/core/src/modules/mdoc/Mdoc.ts

+ this.base64Url = TypedArrayEncoder.toBase64URL(cborEncode(issuerSigned))
+ }
+
+ public static _interalFromIssuerSignedDocument(issuerSignedDocument: IssuerSignedDocument) {


i think if it's public we shouldn't call it _internal, but just fromIssuerSignedDocument. Or we don't allow creating it from IssuerSignedDocument at all (but something else).

TimoGlastra · 2024-10-11T10:46:41Z

packages/core/src/modules/mdoc/MdocOptions.ts

+ /**
+ * The base64Url-encoded device response string.
+ */
+ deviceResponse: string


Why do we need to add both the mdoc and the deviceResponse base64? The encoded device response also contains the mdoc right?

If it's for internal APIs it's ok, but for public APIs I'd like to avoid options that could conflict (what if they don't match?)

TimoGlastra · 2024-10-11T10:48:13Z

packages/core/src/modules/mdoc/MdocOptions.ts

+ deviceResponse: string
+
+ /**
+ * The private part of the ephemeral key used in the session where the DeviceResponse was obtained. This is only required if the DeviceResponse is using the MAC method for device authentication.


This is not the private part. It's a public key that can be used during verificadtion to know which private key to use.

Suggested change

* The private part of the ephemeral key used in the session where the DeviceResponse was obtained. This is only required if the DeviceResponse is using the MAC method for device authentication.

* The public ephemeral key used in the session where the DeviceResponse was obtained. This is only required if the DeviceResponse is using the MAC method for device authentication.

Also, isn't the public key added in the device respnose, or do we need to make sure it's MAC'd to this specific key?

TimoGlastra · 2024-10-11T10:48:41Z

packages/core/src/modules/mdoc/MdocOptions.ts

+ * The trusted base64-encoded issuer certificate string in the DER-format.
+ */
+ issuerCertificate: string
+ holderPublicKey: Key


Either both key, or both PublicKey, but issuerKey and holderPublicKey doesn't make sense I think

Suggested change

holderPublicKey: Key

holderKey: Key

TimoGlastra · 2024-10-11T10:49:44Z

packages/core/src/modules/mdoc/MdocService.ts

+ }
+
+ public async createDeviceResponse(agentContext: AgentContext, options: MdocDeviceResponseOpenId4VpOptions) {
+ return MdocDeviceResponse.openId4Vp(agentContext, options)


Is device response an openid4vp specific object, if we're using BLE don't we use device response? If so I don't think the underlying API should be called openId4Vp?

Or should this method be called createOpenId4VpDeviceResponse?

TimoGlastra · 2024-10-11T10:50:47Z

packages/core/src/modules/mdoc/__tests__/mdoc.deviceResponse.openid4vp.test.ts

nice but usually we name tests mdoc-deviceResponse-openid4vc.test.ts instead of using dots (which are more for extension types)

TimoGlastra · 2024-10-11T10:52:27Z

packages/core/src/modules/mdoc/__tests__/mdoc.deviceResponse.openid4vp.test.ts

+
+ // this is the ISSUER side
+ {
+ mdoc = await Mdoc.create(agent.context, {


do we also want to add an oid4vci flow with mdoc? OR lateR?

TimoGlastra · 2024-10-11T10:54:26Z

packages/core/src/modules/mdoc/__tests__/mdoc.deviceResponse.openid4vp.test.ts

+ ;[
+ [
+ 'clientId',
+ {
+ clientId: 'wrong',
+ responseUri,
+ verifierGeneratedNonce,
+ mdocGeneratedNonce,
+ },
+ ] as const,
+ [
+ 'responseUri',
+ {
+ clientId,
+ responseUri: 'wrong',
+ verifierGeneratedNonce,
+ mdocGeneratedNonce,
+ },
+ ] as const,
+ [
+ 'verifierGeneratedNonce',
+ {
+ clientId,
+ responseUri,
+ verifierGeneratedNonce: 'wrong',
+ mdocGeneratedNonce,
+ },
+ ] as const,
+ [
+ 'mdocGeneratedNonce',
+ {
+ clientId,
+ responseUri,
+ verifierGeneratedNonce,
+ mdocGeneratedNonce: 'wrong',
+ },
+ ] as const,
+ ].forEach(([name, values]) => {


I think this might look a bit clear?

Suggested change

;[

[

'clientId',

{

clientId: 'wrong',

responseUri,

verifierGeneratedNonce,

mdocGeneratedNonce,

},

] as const,

[

'responseUri',

{

clientId,

responseUri: 'wrong',

verifierGeneratedNonce,

mdocGeneratedNonce,

},

] as const,

[

'verifierGeneratedNonce',

{

clientId,

responseUri,

verifierGeneratedNonce: 'wrong',

mdocGeneratedNonce,

},

] as const,

[

'mdocGeneratedNonce',

{

clientId,

responseUri,

verifierGeneratedNonce,

mdocGeneratedNonce: 'wrong',

},

] as const,

].forEach(([name, values]) => {

Object.entries({

clientId: {

clientId: 'wrong',

responseUri,

verifierGeneratedNonce,

mdocGeneratedNonce,

}

}).forEach(([name, values]) => {

TimoGlastra

Nice 👍

auer-martin force-pushed the mdoc-new branch from 2b61629 to a055c94 Compare October 8, 2024 13:56

auer-martin added 3 commits October 8, 2024 15:57

feat: mdoc alpha

8c14678

Signed-off-by: Martin Auer <[email protected]>

feat: fix: some nits

1be0fbf

Signed-off-by: Martin Auer <[email protected]>

docs(changeset): feat: mdoc-support

2fcde60

Signed-off-by: Martin Auer <[email protected]>

auer-martin force-pushed the mdoc-new branch from d7be7a7 to 2fcde60 Compare October 8, 2024 13:57

TimoGlastra reviewed Oct 8, 2024

View reviewed changes

auer-martin added 2 commits October 8, 2024 18:59

Merge branch 'main' of github.com:openwallet-foundation/credo-ts into…

8728d37

… test

fix: some nits

95eb0db

Signed-off-by: Martin Auer <[email protected]>

TimoGlastra reviewed Oct 11, 2024

View reviewed changes

	throw new CredoError('Mdoc verifiable credential is not yet supported.')
	throw new CredoError('Mdoc verifiable presentation is not yet supported.')

		.filter((c) => c instanceof MdocRecord === false)
		.map((c) => getSphereonOriginalVerifiableCredential(c as SdJwtVcRecord \| W3cCredentialRecord))


		const publicDeviceJwk = COSEKey.import(deviceKeyInfo.deviceKey).toJWK()

		deviceKeyInfo.deviceKey

	id: 'MdocPresentationSubmission ' + agentContext.wallet.generateNonce(),
	id: 'MdocPresentationSubmission ' + await agentContext.wallet.generateNonce(),

	* The private part of the ephemeral key used in the session where the DeviceResponse was obtained. This is only required if the DeviceResponse is using the MAC method for device authentication.
	* The public ephemeral key used in the session where the DeviceResponse was obtained. This is only required if the DeviceResponse is using the MAC method for device authentication.

feat: mdoc-support #2054

Are you sure you want to change the base?

feat: mdoc-support #2054

Conversation

auer-martin commented Oct 8, 2024

changeset-bot bot commented Oct 8, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auer-martin Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auer-martin Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Oct 8, 2024 •

edited

Loading

auer-martin Oct 8, 2024 •

edited

Loading

auer-martin Oct 8, 2024 •

edited

Loading